Split out namespace aware reader to a struct that manage the internal namespaces buffer #437

Mingun · 2022-07-23T20:24:02Z

This is PR for low-level namespaces support in the quick-xml, on which high-level serde support will be implemented.

The goals of this PR is to make a safe interface and leave the option of not paying for namespace support if it is not needed. For that the buffer with readed namespaces now managed by the dedicated NsReader reader, which should be used instead of Reader when you need namespaces. Because now buffer is stored inside the reader, the user cannot accidently clear it and get a panic or wrong results later, when it would try to resolve namespace. In addition, it is impossible now to mix read methods with and without namespace awareness, which could lead to hard-to-catch bugs.

Originally, I've planned to include the serde part into this PR, but number of commits for support serde grows and I've decided to made a dedicated PR for that. At least, the serde part slightly depends on #434, because those changes will break some removed tests.

I would like to merge that before #425, because:

it includes the first commit from Split Reader into SliceReader and BufferedReader #425
it makes unnecessary the second commit from Split Reader into SliceReader and BufferedReader #425
I want to try to improve Split Reader into SliceReader and BufferedReader #425 next week

This commit only moves code without significant changes (the only changes is: - corrected imports - add imports to the doc comments which have become inaccessible )

The following commit will not allow to call `reader.decoder()` while reference to ResolveResult is valid, so we'll need to save decoder before the loop and update it when it could be changed

codecov-commenter · 2022-07-23T20:34:38Z

Codecov Report

Merging #437 (45be217) into master (ebbcce0) will increase coverage by 1.99%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master     #437      +/-   ##
==========================================
+ Coverage   49.51%   51.50%   +1.99%     
==========================================
  Files          22       25       +3     
  Lines       13847    13310     -537     
==========================================
- Hits         6856     6855       -1     
+ Misses       6991     6455     -536

Flag	Coverage Δ
unittests	`51.50% <72.72%> (+1.99%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
benches/microbenches.rs	`0.00% <0.00%> (ø)`
src/lib.rs	`12.26% <0.00%> (ø)`
src/name.rs	`88.37% <ø> (ø)`
src/reader/ns_reader.rs	`60.37% <60.37%> (ø)`
src/reader/buffered_reader.rs	`65.06% <65.06%> (ø)`
src/reader/mod.rs	`90.42% <84.00%> (ø)`
src/reader/slice_reader.rs	`100.00% <100.00%> (ø)`
src/de/mod.rs	`77.77% <0.00%> (-0.46%)`	⬇️
src/de/map.rs	`72.55% <0.00%> (+0.46%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebbcce0...45be217. Read the comment docs.

src/reader/mod.rs

src/reader/ns_reader.rs

dralley · 2022-07-24T03:25:38Z

src/reader/ns_reader.rs

+    /// [`End`]: Event::End
+    /// [`read_event()`]: Self::read_event
+    #[inline]
+    pub fn read_event_ns(&mut self) -> Result<(ResolveResult, Event<'i>)> {


I'm not sure this naming is an improvment, it feels too brief. Perhaps read_event_with_ns() or read_event_and_ns() would be better.

I agree, not a better name. May be read_resolved_event?

Changed to read_resolved_event / read_resolved_event_into

src/reader/mod.rs

… to it This commit only moves code and do not introduce any improvements

This commit only moves code

`Deserializer` implementation can buffer events and it will not be able to buffer the `resolve` result because it is bound to the `NsReader`

Co-authored-by: Daniel Alley <dalley@redhat.com>

dralley

I think the PR still needs to be pushed, changes aren't live yet. read_resolved_event() still doesn't feel like the best name - but I have no suggestions that feel clearly better.

…ll non-namespace-aware mutation methods Because `impl DerefMut<Target=Reader> for NsReader` is removed, all Reader's methods that accept `&mut self` won't be available for the NsReader, including configuration methods. Because we need a way to configure our reader, we need to reimplement necessary methods that accepts `&mut self`. In order to not duplicate the code and the doc of this configure methods, they was put into the macro `configure_methods!` and embed to both readers.

dralley · 2022-07-24T13:06:00Z

src/reader/mod.rs

@@ -77,6 +77,19 @@ macro_rules! configure_methods {

        /// Changes whether mismatched closing tag names should be detected.
        ///
+        /// Note, that start and end tags [should match literally][spec], they cannot
+        /// have different prefixes even if both prefixes resolves to the same namespace.
+        /// The


This should be either

The XML

or

This XML:

or

This:

Also "both prefixes resolves" -> "both prefixes resolve"

Done ("The XML") and done!

Co-authored-by: Daniel Alley <dalley@redhat.com>

Mingun added 2 commits July 20, 2022 12:00

Move buffered and borrowing parts of reader to separate files

ded1b77

This commit only moves code without significant changes (the only changes is: - corrected imports - add imports to the doc comments which have become inaccessible )

Merge xmlrs_display into test_bytes

136e77d

The following commit will not allow to call `reader.decoder()` while reference to ResolveResult is valid, so we'll need to save decoder before the loop and update it when it could be changed

Mingun added enhancement namespaces Issues related to namespaces support labels Jul 23, 2022

Mingun requested a review from dralley July 23, 2022 20:24

dralley reviewed Jul 24, 2022

View reviewed changes

src/reader/mod.rs Outdated Show resolved Hide resolved

dralley reviewed Jul 24, 2022

View reviewed changes

src/reader/ns_reader.rs Outdated Show resolved Hide resolved

dralley reviewed Jul 24, 2022

View reviewed changes

src/reader/ns_reader.rs Outdated Show resolved Hide resolved

dralley reviewed Jul 24, 2022

View reviewed changes

Mingun commented Jul 24, 2022

View reviewed changes

src/reader/mod.rs Outdated Show resolved Hide resolved

Mingun and others added 7 commits July 24, 2022 17:39

ns: Introduce an NsReader struct and move all namespace-related API…

f3c0c1b

… to it This commit only moves code and do not introduce any improvements

ns: Make namespace reading independent from buffer

a90f83b

ns: Group unbounded impl blocks

6dd6b53

This commit only moves code

ns: Add API that allow to not resolve namespaces after each event read

6dc166f

`Deserializer` implementation can buffer events and it will not be able to buffer the `resolve` result because it is bound to the `NsReader`

ns: Implement namespace reading for borrowed reader

0759dfd

Co-authored-by: Daniel Alley <dalley@redhat.com>

ns: Rename namespace resolution functions and document them better

16168e1

ns: Add constructors and destructors for parity with Reader

8759d3a

dralley approved these changes Jul 24, 2022

View reviewed changes

Mingun force-pushed the namespaces branch from 4f3b362 to 92a342c Compare July 24, 2022 13:03

dralley reviewed Jul 24, 2022

View reviewed changes

Mingun force-pushed the namespaces branch from 92a342c to dd2ee12 Compare July 24, 2022 13:35

ns: Add a note about rules for closing tags

45be217

Co-authored-by: Daniel Alley <dalley@redhat.com>

Mingun force-pushed the namespaces branch from dd2ee12 to 45be217 Compare July 24, 2022 13:38

dralley merged commit a5da628 into tafia:master Jul 24, 2022

Mingun deleted the namespaces branch July 24, 2022 14:12

Mingun mentioned this pull request Jul 30, 2022

read_event vs. read_namespaced_event #59

Closed

Xiphoseer mentioned this pull request Apr 12, 2024

de: consider local name only for namespaced tags in structs with $value #736

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split out namespace aware reader to a struct that manage the internal namespaces buffer #437

Split out namespace aware reader to a struct that manage the internal namespaces buffer #437

Mingun commented Jul 23, 2022

codecov-commenter commented Jul 23, 2022 •

edited

Loading

dralley Jul 24, 2022 •

edited

Loading

Mingun Jul 24, 2022

Mingun Jul 24, 2022

dralley left a comment

dralley Jul 24, 2022 •

edited

Loading

dralley Jul 24, 2022

Mingun Jul 24, 2022 •

edited

Loading

Split out namespace aware reader to a struct that manage the internal namespaces buffer #437

Split out namespace aware reader to a struct that manage the internal namespaces buffer #437

Conversation

Mingun commented Jul 23, 2022

codecov-commenter commented Jul 23, 2022 • edited Loading

Codecov Report

dralley Jul 24, 2022 • edited Loading

Choose a reason for hiding this comment

Mingun Jul 24, 2022

Choose a reason for hiding this comment

Mingun Jul 24, 2022

Choose a reason for hiding this comment

dralley left a comment

Choose a reason for hiding this comment

dralley Jul 24, 2022 • edited Loading

Choose a reason for hiding this comment

dralley Jul 24, 2022

Choose a reason for hiding this comment

Mingun Jul 24, 2022 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Jul 23, 2022 •

edited

Loading

dralley Jul 24, 2022 •

edited

Loading

dralley Jul 24, 2022 •

edited

Loading

Mingun Jul 24, 2022 •

edited

Loading